Test against symfony versions #90

Nyholm · 2017-11-18T06:33:33Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets
Documentation	if this is a new feature, link to pull request in https://github.com/php-http/documentation that adds relevant documentation
License	MIT

What's in this PR?

I want to test with different versions of the options-resolver.

Why?

Im experimenting to see if the symfony/lts is something that should be used in travis.yml for more bundles.

#SymfonyConHackday2017

Nyholm · 2017-11-18T16:38:06Z

We would like to keep tests for beta and the 2 latest Symfony LTS.

Ping @weaverryan.

This does not quite work.. When Symfony 4.0 is released we do no longer test SF3.4... I will do another attempt later.

Nyholm · 2017-11-18T20:45:10Z

I think I found the "perfect" travis.yml.

The current state of the PR has "symfony/options-resolver": "^2.6 || ^3.0". Travis will test against the following:

SF 2.6 (lowest)
SF 3.3 (Current latest stable)
SF 2.8 (LTS v2)
SF 3.3 (LTS v3) (Same as above)
SF 3.4-beta (Beta) (Allowed to fail)

When Symfony 4 is released and I do not change my library requirements, Travis will test the following:

SF 2.6 (lowest)
SF 3.4 (Current latest stable) <--- Change
SF 2.8 (LTS v2)
SF 3.4 (LTS v3) (Same as above) <--- Change
SF 3.4-beta (Beta) (Allowed to fail) <--- Now meaningless.

If I update my requirements to "symfony/options-resolver": "^2.6 || ^3.0 || ^4.0" after Symfony 4 is released:

SF 2.6 (lowest)
SF 4.0 (Current latest stable) <--- Change
SF 2.8 (LTS v2)
SF 3.4 (LTS v3)
SF 4.0-beta (Beta) (Allowed to fail) <--- Will have a purpose before 4.0 is released

FYI @stof, @nicolas-grekas

nicolas-grekas · 2017-11-18T21:45:23Z

.travis.yml

        - php: 5.4
-          env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" COVERAGE=true TEST_COMMAND="composer test-ci"
-
+          env: DEPENDENCIES="minimun" COVERAGE=true TEST_COMMAND="composer test-ci"


nicolas-grekas · 2017-11-18T21:46:38Z

.travis.yml

+
+          # Test LTS versions
+        - php: 7.1
+          env: DEPENDENCIES="symfony/lts:v2"


clever use of the package, great discovery!

What if some bundle authors actively only want to support 2 Symfony versions at one time? What's what we're talking about in FOSUserBundle FriendsOfSymfony/FOSUserBundle#2639 (comment)

This can still install a mixed set of 2.7 and 2.8 components. What about using symfony/symfony:v2.8.* which will force any symfony component to be in it's v2.8 version?

Sorry, I realize I was unclear! What I really mean was: in FOSUserBundle, they only want to support Symfony 3 and Symfony 4 (and then perhaps just do bug-fixes fro 2.7/2.8 branches).

In that situation, I guess they would just remove this one line and change the minimum dependencies matrix to 5.5 (from 5.4)?

remove the Symfony 2 LTS job and bump your min requirement

@weaverryan they in FOSUserBundle refers to only one maintainer of the bundle actually. I had not even seen this discussion yet (it was hidden in the middle of the thousand notifications I received during the hackday)

re requiring symfony/symfony: that hides problems if we miss depending on some symfony components. if we do that, we should require each component we use in the configured version instead. (which duplicates composer.json, so still not very nice)

We still have the "Test the latest stable release" job which does not alter the composer.json.

Nyholm · 2017-11-19T07:15:35Z

Ping @alcaeus

alcaeus · 2017-11-20T06:24:48Z

.travis.yml

+        - php: 7.1
+
+          # Test LTS versions
+        - php: 7.1


Food for thought: test the LTS releases against the lowest common version they and this package supports. I'd say that people on LTS releases are less likely to always run on the latest PHP version, so testing against a lower version might be more comparable to real-world usage.

weaverryan · 2017-11-20T15:02:23Z

.travis.yml

+    allow_failures:
+          # Latest beta is allowed to fail.
+        - php: 7.1
+          env: DEPENDENCIES="beta"


I think the latest beta should not be allowed to fail. Probably we really should allow it to fail, but I'm afraid people will not "notice" the failure. I'd rather push them to be a bit more proactive by noticing it :)

stof · 2017-11-21T15:56:35Z

.travis.yml

+        - php: 7.1
+          env: DEPENDENCIES="symfony/lts:v3"
+
+          # Latest beta release


I suggest testing against dev rather than beta (especially if you allow them to fail):

if something breaks in an early Symfony 4.1 version, you may have 5 months to report it or adapt, instead of a few weeks/days

most of your other deps don't release beta at all, so you will not catch anything until it reaches other jobs too

dbu

excellent, thanks for this cleanup!

lowest version build fails. i guess that is something about composer.json, maybe phpspec itself?

(and lets drop php 5.4 and 5.5 in the next minor version ;-) )

stof · 2017-11-27T11:31:44Z

.travis.yml

@@ -27,18 +19,44 @@ branches:
 matrix:
    fast_finish: true
    include:
+          # Minimum supported PHP and Symfony version


Minimum supported PHP is confusing here

stof · 2017-11-27T11:32:43Z

.travis.yml


+    allow_failures:
+          # Latest beta is allowed to fail.


wrong comment

stof · 2017-11-27T11:33:16Z

.travis.yml


+    allow_failures:
+          # Latest beta is allowed to fail.
+        - php: 7.1


no need to specify the PHP version here. It will avoid having to keep it in sync when changing the version used by DEPENDENCIES="dev"

stof · 2017-11-27T11:33:54Z

.travis.yml

@@ -27,18 +19,44 @@ branches:
 matrix:
    fast_finish: true
    include:
+          # Minimum supported PHP and Symfony version
+        - php: 7.1
+          env: DEPENDENCIES="minimum" COVERAGE=true TEST_COMMAND="composer test-ci"


should we actually run the coverage on the min deps ?

i'd do coverage on the latest stable

stof · 2017-11-27T11:34:16Z

src/Plugin/ContentTypePlugin.php

@@ -102,7 +102,7 @@ private function isJson($stream)

        json_decode($stream->getContents());

-        return json_last_error() == JSON_ERROR_NONE;
+        return JSON_ERROR_NONE == json_last_error();


strict comparison ?

xabbuh · 2017-11-29T20:58:16Z

.travis.yml

 before_install:
    - if [[ $COVERAGE != true ]]; then phpenv config-rm xdebug.ini || true; fi
+    - if [ "$DEPENDENCIES" = "minimum" ]; then COMPOSER_FLAGS="--prefer-stable --prefer-lowest"; fi;


I would instead just set $COMPOSER_FLAGS above

xabbuh · 2017-11-29T20:58:37Z

.travis.yml

 before_install:
    - if [[ $COVERAGE != true ]]; then phpenv config-rm xdebug.ini || true; fi
+    - if [ "$DEPENDENCIES" = "minimum" ]; then COMPOSER_FLAGS="--prefer-stable --prefer-lowest"; fi;
+    - if [ "$DEPENDENCIES" = "dev" ]; then composer config minimum-stability dev; fi;


I would rename the var to $STABILITY (or something like that) here

xabbuh · 2017-11-29T20:58:53Z

.travis.yml

 before_install:
    - if [[ $COVERAGE != true ]]; then phpenv config-rm xdebug.ini || true; fi
+    - if [ "$DEPENDENCIES" = "minimum" ]; then COMPOSER_FLAGS="--prefer-stable --prefer-lowest"; fi;
+    - if [ "$DEPENDENCIES" = "dev" ]; then composer config minimum-stability dev; fi;
+    - if [[ $DEPENDENCIES == *"/"* ]]; then composer require --no-update $DEPENDENCIES; fi;


and then just test for $DEPENDENCIES not being empty here

Nyholm · 2017-11-29T21:04:06Z

Thank you for the review. An Im sorry for keeping you review the actual travis file here and the "best practice" in the docs.

I've made the updates now

fbourigault · 2017-11-30T10:20:23Z

.travis.yml

@@ -27,18 +19,46 @@ branches:
 matrix:
    fast_finish: true
    include:
+        - php: 7.1
+          env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" DEPENDENCIES="doctrine/instanciator:^1.05"


It's 1.0.5 ;)

alcaeus · 2017-11-30T10:25:36Z

.travis.yml

@@ -27,18 +19,46 @@ branches:
 matrix:
    fast_finish: true
    include:
+        - php: 7.1
+          env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" DEPENDENCIES="doctrine/instanciator:^1.0.5"


It's also instantiator. Apart from that, any reason why you're locking the version of it? If anything breaks with --prefer-lowest because an older version is installed, it needs to be fixed in that package and should probably be added here as a comment so the workaround can be removed.

this is adding the dependency for this one case, so its changing composer.json and thus needs a version constraint.

I can see that, the question is: why do you need it as an explicit dependency?

From Slack:

@fbourigault [11:18 AM]
$ composer why doctrine/instantiator
phpspec/phpspec 2.4.0 requires doctrine/instantiator (^1.0.1)
phpspec/prophecy 1.4.0 requires doctrine/instantiator (^1.0.2) (edited)
But it’s for unmaintained versions of those packagesages

I see. Thank you for explaining 👍

fbourigault · 2017-11-30T10:40:38Z

.travis.yml

@@ -27,18 +19,47 @@ branches:
 matrix:
    fast_finish: true
    include:
+        - php: 7.0
+          env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" DEPENDENCIES="doctrine/instantiator:^1.1"


doctrine/instantiator:^1.1 require PHP 7.1.

Nyholm · 2017-11-30T10:58:58Z

Thank you for all the reviews!

…luz) This PR was submitted for the master branch but it was merged into the 4.0 branch instead (closes #8701). Discussion ---------- Added a good example of a travis file After a lot of discussions with people at Symfony con I came up with a "perfect" travis file. See php-http/client-common#90 (comment) for info. Btw, should I make a table explaining what we really test? (Like on the client-common PR) Commits ------- 93bdb86 Rewords 4bd8e65 Minor updates bc9a43f use ./ before vendor ae322b7 typo 1017897 Using better cache syntax 36ca90b Using PHPUNIT_FLAGS 18e4a18 Added back php-versions 05bf09d Moved coverage to PHP 7.2 with latest deps bfdd064 Fixed minor things 3e2b876 Fixed typos and created more variables a62bcb7 Added comment and install simple-phpunit deps 65f91df Use tilde or composer validate will never pass 179abb0 Updated according to feedback a281604 Typo 2418834 Added a good example travis file

Nyholm added 2 commits November 18, 2017 08:32

Test agains symfony versions

bae14a7

cs fixes

1cb572b

Nyholm mentioned this pull request Nov 18, 2017

conflict rules for v3 should also forbid a v2 package symfony/lts#3

Closed

Nyholm added 3 commits November 18, 2017 18:22

Improve travis tests

f1cb956

Allow failures on BETA

2f59b40

typo

092d832

Nyholm added 4 commits November 18, 2017 22:23

Test LTS releases

37d3698

typos

7920c09

Check if string contains slash

f066efb

Remove Symfony4.0 to see how travis behaves

14df844

nicolas-grekas reviewed Nov 18, 2017

View reviewed changes

Nyholm added 2 commits November 19, 2017 14:08

Make the PR ready for merge

8a5122a

typo

566ad7f

This was referenced Nov 19, 2017

Support for Symfony 4 sjparkinson/static-review#68

Closed

Added a good example of a travis file symfony/symfony-docs#8701

Closed

alcaeus reviewed Nov 20, 2017

View reviewed changes

weaverryan reviewed Nov 20, 2017

View reviewed changes

stof reviewed Nov 21, 2017

View reviewed changes

Nyholm added 2 commits November 25, 2017 10:02

Updated according to feedback

24ed526

Use tilde

5d04ded

Nyholm mentioned this pull request Nov 25, 2017

Update dependency for Symfony 4. php-http/stopwatch-plugin#4

Merged

dbu changed the title ~~Test agains symfony versions~~ Test against symfony versions Nov 27, 2017

dbu approved these changes Nov 27, 2017

View reviewed changes

stof suggested changes Nov 27, 2017

View reviewed changes

Nyholm added 2 commits November 29, 2017 20:44

Make sure tests passes

2a71c9b

typo

88594a1

Do not run coverege on min-deps

901d830

xabbuh reviewed Nov 29, 2017

View reviewed changes

Nyholm added 2 commits November 29, 2017 22:02

Introduce stability variable

73d4ede

typo

921ddba

Nyholm added 5 commits November 30, 2017 11:05

Fixes

518f86b

Added back PHPSpec 2

e9e9073

fixed PHP versions

8d1403b

Merge branch 'master' into Nyholm-patch-1

bceddc0

Fixed deps low

8c62d22

fbourigault reviewed Nov 30, 2017

View reviewed changes

Fixed typo

dcb2bca

alcaeus reviewed Nov 30, 2017

View reviewed changes

fbourigault approved these changes Nov 30, 2017

View reviewed changes

Minor

9d5c846

dbu approved these changes Nov 30, 2017

View reviewed changes

Test on rc

dc97863

fbourigault reviewed Nov 30, 2017

View reviewed changes

PHP 7.1

03ca9ce

Nyholm merged commit fcf5989 into master Nov 30, 2017

Nyholm deleted the Nyholm-patch-1 branch November 30, 2017 10:59

Test against symfony versions #90

Test against symfony versions #90

Uh oh!

Conversation

Nyholm commented Nov 18, 2017

What's in this PR?

Why?

Uh oh!

Nyholm commented Nov 18, 2017

Uh oh!

Nyholm commented Nov 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Nov 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Nov 30, 2017

Uh oh!

Uh oh!